Fix Watershed class: persist instance state and parse JSON#245
Fix Watershed class: persist instance state and parse JSON#245thodson-usgs wants to merge 8 commits into
Conversation
…ershed `Watershed.from_streamstats_json` was a classmethod that assigned to `cls` instead of an instance, so every watershed shared class-level attributes and the method returned the class itself. `Watershed.__init__` called `get_watershed(...)` and discarded the result, so instances had no state of their own. And `get_watershed(format="object")` returned `None` rather than the parsed JSON the docstring promised. `from_streamstats_json` now builds a real instance, `__init__` requests `format="object"` and populates `self`, and `get_watershed(format="object")` returns the parsed dict. `_workspaceID` is preserved as a read-only alias of the new `workspace_id` attribute. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes dataretrieval.streamstats.Watershed construction/parsing so instances hold their own state and get_watershed(format="object") returns parsed JSON as advertised, with new regression tests to prevent class-level state leaks.
Changes:
- Make
get_watershed(format="object")return a parsed JSONdict(instead ofNone) and reuse the parsed data forWatershedcreation. - Rework
Watershed.from_streamstats_jsonto construct and populate a real instance (no class-level mutation), and makeWatershed.__init__populate instance attributes. - Add
tests/streamstats_test.pycovering the regressions and the_workspaceIDback-compat alias.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
dataretrieval/streamstats.py |
Fixes JSON parsing/return behavior and corrects Watershed instance construction/state population. |
tests/streamstats_test.py |
Adds regression tests for instance state, object-format return, init population, and _workspaceID aliasing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data = json.loads(r.text) | ||
|
|
||
| if format == "object": | ||
| # return a python object | ||
| pass | ||
| return data | ||
|
|
||
| data = json.loads(r.text) | ||
| return Watershed.from_streamstats_json(data) | ||
|
|
Per copilot review on PR DOI-USGS#245. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per /simplify review on PR DOI-USGS#245: - Replace `json.loads(r.text)` with `r.json()` (matching the convention used in `nldi.py` and `nwis.py`) and drop the now-unused `import json` from `dataretrieval/streamstats.py`. - Remove the dead `format == "shape"` branch — a stale Fiona-stub (`# use Fiona to return a shape object` + `pass`) that silently fell through to `Watershed.from_streamstats_json(data)`. The docstring no longer mentions `"shape"` so removing the branch matches the contract. - Drop four trivial unit tests that each exercised a one-line behavior change obvious from the diff (`from_streamstats_json` returns an instance, `format="object"` returns a dict, `__init__` populates self, `_workspaceID` alias resolves). Keep the load-bearing one: `test_from_streamstats_json_does_not_mutate_class` guards against re-introducing the class-mutation antipattern under refactor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously `get_watershed` had two named formats (`"geojson"`, `"object"`) and any other value silently fell through to a `Watershed` — the same bug shape that masked the dead `"shape"` branch removed earlier in this PR. Add an explicit `"watershed"` case and raise `ValueError` on unrecognized values so a typo'd format fails loudly instead of returning whatever the fallthrough is. Also fix `get_sample_watershed`, whose docstring promises a `Watershed` but called `get_watershed(...)` with the default `format="geojson"` (returns the raw `requests.Response`). Pass `format="watershed"` explicitly so the function matches its documented return type. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| format: string, optional | ||
| Selects the return shape. ``"geojson"`` (default) returns the raw | ||
| ``requests.Response``; ``"object"`` returns the parsed JSON ``dict``; | ||
| ``"watershed"`` returns a :obj:`Watershed` instance built from the | ||
| parsed JSON. Any other value raises ``ValueError``. |
| if format == "geojson": | ||
| return r | ||
|
|
||
| if format == "shape": | ||
| # use Fiona to return a shape object | ||
| pass | ||
| data = r.json() | ||
|
|
||
| if format == "object": | ||
| # return a python object | ||
| pass | ||
| return data | ||
|
|
||
| if format == "watershed": | ||
| return Watershed.from_streamstats_json(data) | ||
|
|
||
| data = json.loads(r.text) | ||
| return Watershed.from_streamstats_json(data) | ||
| raise ValueError( | ||
| f"Invalid format {format!r}; expected 'geojson', 'object', or 'watershed'." | ||
| ) |
Per Copilot review: the narrative section claimed `get_watershed` "Returns a watershed object", but with the explicit `format` contract the function actually returns a `requests.Response` (default), a `dict`, or a `Watershed` depending on `format`. Rewrite the narrative to describe the three return shapes accurately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
b3a5d29 to
e9aa572
Compare
- `_workspaceID` now matches the pattern used by `NWIS_Metadata.variable_info` (nwis.py:1190-1202): aliased attributes warn with stacklevel=2 pointing to the new spelling, so the alias can be removed in a future minor without surprising users. - Add the four cheap tests the PR description called out as "trivial": - `from_streamstats_json` returns a `Watershed` instance (not the class) - `_workspaceID` aliases `workspace_id` and emits DeprecationWarning - `format="object"` returns the parsed JSON dict - `format="watershed"` returns a `Watershed` instance - unknown `format` raises `ValueError` The new tests share a `patched_get` fixture that stubs `requests.get` with a `SAMPLE_JSON`-returning mock — no network, no new fixtures dir. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Closing as superseded. The |
Summary
Watershed.from_streamstats_jsonwas a classmethod that mutatedclsinstead of building an instance, causing every watershed to share class-level state and the method to return the class itself rather than aWatershed.Watershed.__init__(rcode, x, y)calledget_watershed(...)and discarded the result, so a freshly-constructed instance had no attributes of its own.get_watershed(format="object")returnedNoneinstead of the parsed JSON dict its docstring promised.This PR makes
from_streamstats_jsonreturn a real instance, makes__init__populateselfviaget_watershed(format="object"), and makesformat="object"actually return the parsed JSON. The legacy_workspaceIDattribute is preserved as a read-only alias for the newworkspace_id.While here:
format == "shape"branch (a stale Fiona-stubpassthat fell through to aWatersheddespite documenting a different return type).formatcontract explicit: added a"watershed"case alongside"geojson"/"object", and unrecognized values now raiseValueError. Previously any unrecognized format silently fell through to aWatershed— the same bug shape that masked the broken"shape"branch.get_sample_watershedto actually return aWatershedinstance (preexisting docstring/behavior mismatch — it calledget_watershed(...)with the defaultformat="geojson"and so returned the rawResponse).json.loads(r.text)tor.json()to match the convention used innldi.py/nwis.py.get_watershednarrative docstring to describe the multi-return contract (Response/dict/Watershedselected byformat) instead of the previous "always returns a watershed" wording, which contradicted the actual default behavior.Minimal reproducible examples
Reconstructed inline because the live StreamStats endpoint is currently returning 404 across requests; the bug shapes are pure-Python and don't depend on the API.
Bug 1:
from_streamstats_jsonmutates the classBug 2:
__init__discards the workBug 3:
format="object"returnsNoneAfter this PR,
format="object"returns the parseddict,Watershed.from_streamstats_jsonreturns a fresh instance with isolated state, andWatershed(...)populatesself.Test plan
tests/streamstats_test.pycovering the load-bearing behavior change: two watersheds built from different JSON must not share state via class-level attributes (test_from_streamstats_json_does_not_mutate_class). The other behaviors —from_streamstats_jsonreturns an instance,format="object"returns a dict,__init__populatesself,_workspaceIDresolves — are each one-line obvious fixes; their tests were dropped as trivial.watershed.geojsonendpoint is currently returning HTTP 404 for all tested coordinates (service-side issue independent of this PR). Suggested follow-up: re-run the existingtest_get_watershedintegration once StreamStats recovers.Follow-up
Filed a separate issue to discuss whether
get_watershedshould always return aWatershed(drop theformatparameter). That would match the function name and the original narrative-docstring promise, but is a breaking change and out of scope for this bug-fix PR.